Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Stylus configure dialog height by refactoring and Optimizing Stylus Popup Height Adjustment Logic #1808

Merged
merged 2 commits into from
Aug 18, 2024
Merged

Fix Stylus configure dialog height by refactoring and Optimizing Stylus Popup Height Adjustment Logic #1808

merged 2 commits into from
Aug 18, 2024

Conversation

Ameer-Jamal
Copy link
Contributor

@Ameer-Jamal Ameer-Jamal commented Aug 18, 2024 •
edited
Loading

PR Description: Fix for Proper Popup Resizing by Changing CSS initial to revert

Summary:
This PR addresses an issue where the popup resizing logic was not functioning as expected due to the use of the CSS property all:initial within the onWindowResize function. The fix involved changing this to all:revert, which led to the correct behavior.

Key Change:

  • CSS Adjustment:
    • Changed the CSS style applied to the temporary #max element from all:initial to all:revert.
    • This change allows the element to revert to its expected styles, improving compatibility with the existing layout and preventing unintended style overrides.

Impact:

  • Corrected Resizing Behavior: The popup now resizes correctly on window resize events, ensuring a better user experience.
  • Enhanced Compatibility: Using revert ensures the element respects inherited or user-agent styles, which can be more predictable and reliable than initial.

Conclusion:
This fix resolves the issue with popup resizing by leveraging the more appropriate revert CSS property, leading to more consistent behavior across different environments.

@Ameer-Jamal Ameer-Jamal changed the title Master Fix Stylus configure dialog height by refactoring and Optimizing Stylus Popup Height Adjustment Logic Aug 18, 2024
@Ameer-Jamal Ameer-Jamal mentioned this pull request Aug 18, 2024
Closed
@Ameer-Jamal
Copy link
Contributor Author

also the height isnt over done so you can scroll within the configure dialog

@Ameer-Jamal
Copy link
Contributor Author

@tophf Can you check this, thank you!

@tophf
Copy link
Member

tophf commented Aug 18, 2024

The problem looks like #1795 which is already fixed in the source code, but it wasn't released yet. Could you test it?

@Ameer-Jamal
Copy link
Contributor Author

@tophf I will test it , what branch is the fix on ?

@tophf
Copy link
Member

tophf commented Aug 18, 2024

It's already in the default branch.

@Ameer-Jamal
Copy link
Contributor Author

@tophf Well in that case I forked master (which im assuming is the default) a couple hours ago, and the problem was still present

@tophf
Copy link
Member

tophf commented Aug 18, 2024

What's your OS, OS scaling, browser zoom in browser settings, extension zoom in Stylus style manager page?

@Ameer-Jamal
Copy link
Contributor Author

Ameer-Jamal commented Aug 18, 2024 •
edited
Loading

What's your OS, OS scaling, browser zoom in browser settings, extension zoom in Stylus style manager page?

@tophf
Im on windows, I also have mac and have noticed the same problem, im on a 4k 32 inch screen my browser is edge and im usually on 100% zoom, I've tested multiple browsers zoom levels it makes no difference

Windows zoom level and mac I have them at 125%

@tophf
Copy link
Member

tophf commented Aug 18, 2024

My display is single density and I can't reproduce the problem at 125%. Anyway, there should be no need to do what you did in this PR and recalculate positions. The problem should be fixed in our existing functions: adjustSizeForPopup (config-dialog.js) and onWindowResize (popup.js).

@tophf
Copy link
Member

tophf commented Aug 18, 2024

I see there was a related bug in our code. Try changing all:initial to all:revert in popup.js line 85.

@tophf
Copy link
Member

tophf commented Aug 18, 2024

Also please check max-height in the popup's <body> element in devtools i.e. right-click inside the popup and select "inspect" in the menu.

@Ameer-Jamal
Copy link
Contributor Author

Ameer-Jamal commented Aug 18, 2024 •
edited
Loading

@tophf Oh i see yes I just tested and making all:intial go to all:revert fixed the issue !
My max height in body is

 min-width: 325px !important;
 min-height: 543.636px !important;

@Ameer-Jamal
Copy link
Contributor Author

@tophf I removed my change and updated the change

tophf
tophf reviewed Aug 18, 2024
View reviewed changes
package.json Outdated Show resolved Hide resolved
@Ameer-Jamal
Copy link
Contributor Author

@toph my mistake those pesky json files commited by mistake i fixed it

@tophf tophf merged commit 2333b02 into openstyles:master Aug 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tophf tophf tophf left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Ameer-Jamal @tophf